-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Resolve errors blocking usage in a mountable Rails engine #1694
Conversation
91bf560
to
db2b8ab
Compare
db2b8ab
to
50bd90b
Compare
Thanks @kevindew. Could you please rebase with master? |
@gauravtiwari Sure, just done so. |
Thanks Kevin, will take a look at this over the weekend :) |
Please can you rebase again 🙏 |
Anything we can do to help this one along? |
26abb1d
to
a48b1bc
Compare
I've just rebased this now - sorry @gauravtiwari I missed your previous notification. @mathewbaltes if you have some time it'd be useful for you to just try this out and check it all still plays nice. It has been quite a while since I first raised this, but all still seemed to be working good for me giving it a run just now. |
Dir.chdir(Rails.root) do | ||
say "Installing all angular dependencies" | ||
run "yarn add core-js zone.js rxjs @angular/core @angular/common @angular/compiler @angular/platform-browser @angular/platform-browser-dynamic" | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kevindew Do we need to do this? Is it not possible to use root package.json and yarn as a source of truth? But I see, what you mentioned in the description, in case someone wants to publish an engine package to npm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest it's preferable to do this as it keeps the installation consistent to the Rails root (this would be a piece in a different place) and also if you have just a single package.json for both the Rails engine itself and the dummy app then they need to share dependencies when this may not makes sense (e.g. you wouldn't want to have a dependency on webpacker for your distributed engine JS files).
A nice thing though is that if you don't have a package.json in your Rails root then this will just use the global one for your application. So this dir change only prefers a package.json in your mounted app and will fallback to the root one if it doesn't exist.
Thanks @kevindew , no problem 👍 🙏 |
@gauravtiwari Any chance we could look at this again soon? I notice Rails 6 is coming rather soon. |
I'm looking for creating an engine using Vue.js and looking for advices on the setup
It work but it's the right way to do that ? |
Just following up on a comment from @kevindew to @gauravtiwari with the release of Rails 6 and making Webpacker the default answer for JS does this mean that without this getting merged that Webpacker wouldn't be able to work with engines correctly in Rails 6? |
It would be great to see this merged. Thanks @kevindew @gauravtiwari 👍 |
Hello @kevindew Thanks for working on this and apologies for long silence 🙏 very busy at work. Could you please rebase and ping me here for review? |
This changes the assumption that these tasks are being run in the rails root to explicitly determine the binstub directory based on the Rails root. This fixes the issue for when the tasks are being run from a mounted dummy app as part of a Rails engine. Given a fresh Rails plugin: `rails plugin new rails-plugin --mountable` that has webpacker gem installed. Before this change running running `app:webpacker:check_binstubs` would check in `./bin` for webpack binstubs whereas they are actually expected in `test/dummy/bin`. After this change the binstubs for the rails app in `test/dummy/bin` are checked as expected.
If you have a scenario where a Rails apps tasks are placed under a namespace these tasks that invoke other tasks don't work. The common scenario where this happens is when a Rails engine is mounted in a dummy app as per: https://github.com/rails/rails/blob/master/railties/lib/rails/tasks/engine.rake#L4-L17 This change resolves the issues where you receive an error of task not found when running an app command e.g. app:assets:precompile. If we are able to know a task dependency prior to runtime we don't have to do anything complex as per the change for `clobber` since it being defined outside a block does not require a namespace. However for the tasks that we need to perform actions at runtime we have to work out the prefix of the task that was executed to then re-use this on the tasks we want to execute.
a48b1bc
to
69afab1
Compare
When this gem is running inside a Rails engine to do a task such as `app:assets:precompile` an error is thrown: ``` ➜ rails-plugin git:(master) ✗ rake app:assets:precompile Compiling… Compilation failed: /Users/kevindew/dev/webpacker/lib/webpacker/railtie.rb:6:in `<top (required)>': uninitialized constant Rails::Engine (NameError) from /Users/kevindew/dev/webpacker/lib/webpacker.rb:38:in `require' from /Users/kevindew/dev/webpacker/lib/webpacker.rb:38:in `<top (required)>' from /Users/kevindew/dev/rails-plugin/test/dummy/bin/webpack:13:in `require' from /Users/kevindew/dev/rails-plugin/test/dummy/bin/webpack:13:in `<main>' ``` Explicitly requiring rails/engine resolves this.
This resolves problems that occur when the expectation is that the Rails.root is the same as the working directory of the rakefile. A common situation for this is when you have a mounted Rails engine in the test/dummy app to run your engine. This changes the yarn commands to run in the directory where you would have the package.json for the rails app that would be used for mounting rather than the root of the engine. And the node_modules are also installed within that directory so they are in the expected place for tasks such as app:assets:precompile.
The contents of the gitignore only really make sense for the root directory of a Rails application. If we are dealing with a Rails engine that mounts the engine onto a test one for usage then it does not make sense to update the root .gitignore and if someone does have one in the `test/dummy` directory then that one should be updated instead.
This is resolution to problems that occur with a mounted Rails engine. With one of these you would want to run `rake app:webpacker:info` and have it run against the Rails app you'd be mounting in (normally in test/dummy) however it would incorrectly run in the root directory - where you probably don't want to have webpacker and webpacker-dev-server installed.
For a mounted Rails engine we need the config to be in the application mount directory e.g. test/dummy/config/webpacker.yml By running this based on pwd we get this in a Rails engine app: ``` ➜ rails-plugin rake app:webpacker:verify_install RAILS_ENV=development environment is not defined in config/webpacker.yml, falling back to production environment Configuration test/dummy/config/webpacker.yml file not found. Make sure webpacker:install is run successfully before running dependent tasks ``` Whereas in a normal Rails installation it outputs unchanged: ``` ➜ full-app git:(master) ✗ rake webpacker:verify_install RAILS_ENV=development environment is not defined in config/webpacker.yml, falling back to production environment Configuration config/webpacker.yml file not found. Make sure webpacker:install is run successfully before running dependent tasks ```
This sets up a barebones mounted Rails engine so that tests can be performed against it. This follows a similar approach to the other rake tasks test and thus doesn't intend to be too exhaustive in what is testing and is mostly a cursory test that one of the tasks does perform differently in a mounted Rails engine context.
69afab1
to
32a7749
Compare
Hey @gauravtiwari this is now rebased. Let me know when you've had a chance to do a review. I've done a pretty simple rebase here without going through exactly how the gem has changed so there may be some issues to resolve. So once you let me know you're happy with the approach and we're close to merging I'll carve out some time to do a sweep. |
Guys, any update on this? |
@gauravtiwari - any chance this can be reviewed and merged soon? thanks! |
This addresses #1124 and #348 and is related to this PR: rails/rails#33819
This intends to improve support for usage with a mountable Rails engine by fixing the various errors a user encounters running the rake tasks.
A mountable Rails engine tends to include a full Rails app inside a
test/dummy
directory which can be used to test mounting the engine. This is a somewhat unusual situation where you have a Rails app nested within directories from the Rakefile and all the Rails tasks are prefixed withapp
. Thus, these changes mostly solves those issues by running binstubs and yarn commands from the context of theRails.root
, rather than the root directory of the engine and checking whether a prefix is needed to run tasks.An example of usage in a mountable Rails engine
Given we create an engine with
rails plugin new rails-plugin --mountable
we then have a directorytest/dummy
where a new application has been created to allow us to test our engine.If we run
rake app:webpacker:install
this will create binstubs intest/dummy/bin
, config intest/dummy/config
, update thetest/dummy/package.json
, etc - and then run yarn from within the context oftest/dummy
.If we run
rake assets:precompile
it will look for packs intest/dummy/javascript/packs
(or wherever else you configure it too) and output them totest/dummy/public/packs
. Which is consistent with how sprockets would output assets totest/dummy/public/assets
.How this affects engine usage and distribution
The fixes here aren't themselves opinionated - they merely maintain a pattern introduced by other Rails tasks - but it does offer a chance to think about engine usage:
package.json
file which pulls in@rails/webpacker
and can link to the root module via a relative import (as developers shouldn't push@rails/webpacker
out as a dependency when it's not needed - and they shouldn't mark it as a dev dependency if they want to testassets:precompile
)However these changes don't enforce working in this way and a second package.json isn't necessary as yarn will just fall back to the root one.
Hope this all makes sense - there's more info in the commit messages.